Added empty result set check to nextRowset() #520

david-puglielli · 2017-09-07T23:23:53Z

This is a fix for the bug discussed in issue #507.

coveralls · 2017-09-07T23:44:44Z

Coverage decreased (-0.003%) to 74.249% when pulling 83631cd on david-puglielli:nextrowset-fix into 9e695d2 on Microsoft:dev.

coveralls · 2017-09-08T02:37:19Z

Coverage increased (+0.08%) to 74.332% when pulling f4117a8 on david-puglielli:nextrowset-fix into 9e695d2 on Microsoft:dev.

yitam · 2017-09-08T17:07:55Z

source/pdo_sqlsrv/pdo_stmt.cpp

+        CHECK_CUSTOM_ERROR( has_fields == 0, driver_stmt, SQLSRV_ERROR_NO_FIELDS ) {
+            throw core::CoreException();
+        }
+


From this page it seems valid and accurate to call this without making another trip to the server. Please confirm.

Since a statement has to be executed for the user to call nextRowset, there is no trip to the server made here.

Is there an equivalent scenario in the sqlsrv side?
Based on your comment it seems that fetch() also invokes SQLNumResultCols(), right? Would that be a duplicate?

Yes, I've verified the same problem exists on the sqlsrv side. But I think core_sqlsrv_fetch is never called if nextRowset is called before fetch, which is why we're catching the error here. I'll add a fix and test for sqlsrv.

david-puglielli · 2017-09-22T00:57:57Z

Moved the error check to core_sqlsrv_next_result in core_stmt.cpp.

Edit: Moved it back because it was causing test failures.

coveralls · 2017-09-22T01:12:42Z

Coverage decreased (-1.3%) to 72.967% when pulling 8666ea6 on david-puglielli:nextrowset-fix into 9e695d2 on Microsoft:dev.

coveralls · 2017-09-22T04:27:15Z

Coverage decreased (-0.8%) to 73.406% when pulling 2c28d0b on david-puglielli:nextrowset-fix into 9e695d2 on Microsoft:dev.

coveralls · 2017-09-22T04:48:16Z

Coverage decreased (-0.8%) to 73.406% when pulling f11c208 on david-puglielli:nextrowset-fix into 9e695d2 on Microsoft:dev.

…nt error message

coveralls · 2017-09-27T01:53:49Z

Coverage increased (+0.09%) to 73.366% when pulling 66a12d3 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-28T01:22:08Z

Coverage increased (+0.05%) to 73.321% when pulling d56446c on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-28T22:12:51Z

Coverage increased (+0.003%) to 73.276% when pulling 257b1cd on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-28T22:54:47Z

Coverage increased (+0.003%) to 73.276% when pulling 7315d44 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

yukiwongky · 2017-09-29T18:49:50Z

source/pdo_sqlsrv/pdo_stmt.cpp

+            CHECK_CUSTOM_ERROR( !has_result, driver_stmt, SQLSRV_ERROR_NO_FIELDS ) {
+                throw core::CoreException();
+            }
+        }


I don't quite understand. I thought PSOStatement::nextRowset should only be called on a fetched statement. The condition for if says fetched is NOT called, and throws an exception if there is no result. How can there any result if the statement was never fetched?

nextRowset() should only be called on a fetched statement, but there is nothing stopping the user from calling nextRowset immediately after executing, and we need to handle that case. In issue 507 it occurs when going into the 'else' part of stored procedure that returns a result set in the 'if' but not the 'else' branch, so it can happen that user does call nextRowset first accidentally via a stored procedure.

This code catches the case where nextRowset() is called before fetch on a null result set, giving the appropriate error message; before, there would be no error given. In the case where nextRowset is called before fetch on non-null result set, core_sqlsrv_has_any_result will return true and the error is not triggered. In the event that nextRowset is called after fetch() and the result set is null, that will be handled in core_sqlsrv_next_result() (or by the check for past_next_result_end above). I think that covers all the relevant cases...

yea I was confused what happens when fetch isn't called and has_result is true, but now it's more clear. Can you put a comment saying if fetch_called is false and has_result is true, core_sqlsrv_next_result will handle it.

yukiwongky · 2017-09-29T18:56:48Z

test/functional/pdo_sqlsrv/pdo_empty_result_error.phpt

+$conn = new PDO( "sqlsrv:Server = $server; Database = $databaseName; ", $uid, $pwd );
+$conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
+
+$stmt = $conn->query("IF OBJECT_ID('TestEmptySetTable', 'U') IS NOT NULL DROP TABLE TestEmptySetTable");


There a DropTable and DropProc function you can use in MsCommon

coveralls · 2017-09-29T21:46:05Z

Coverage decreased (-0.04%) to 73.23% when pulling 494527d on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-29T22:06:28Z

Coverage increased (+0.003%) to 73.276% when pulling dc6e2a3 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-29T22:26:44Z

Coverage increased (+0.003%) to 73.276% when pulling dc6e2a3 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-29T23:30:15Z

Coverage increased (+0.003%) to 73.276% when pulling 4674f58 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-09-29T23:31:05Z

Coverage increased (+0.003%) to 73.276% when pulling 4674f58 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

yitam

Perhaps add a couple of tests on prepare (not query / exec) on a regular SELECT query (not a stored procedure) then fetch next rowset, with and without client buffers?

yitam · 2017-09-30T00:04:32Z

test/functional/pdo_sqlsrv/pdo_empty_result_error.phpt

+--TEST--
+Test that calling nextRowset() on an empty result set produces the correct error message. Fix for Github 507.
+--SKIPIF--
+<?php require('skipif.inc'); ?>


Please try to limit the length of test title. If you have more please put it in --DESCRIPTION-- instead.
For tests, it's a good practice to drop test tables / procedures at the end to leave no trace.

I do drop them at the end, but I kept the drop functions at the beginning too just in case...

ahhh... I see! In the beginning you use DropTable and DropProc but later you simply use queries.

yitam · 2017-09-30T00:07:00Z

test/functional/sqlsrv/sqlsrv_empty_result_error.phpt

+$stmt = sqlsrv_query($conn, "DROP TABLE TestEmptySetTable");
+$stmt = sqlsrv_query($conn, "DROP PROCEDURE TestEmptySetProc");
+sqlsrv_close($conn);
+?>


just for practice, probably free the statement object too before closing the connection?

yitam · 2017-09-30T00:07:22Z

test/functional/sqlsrv/sqlsrv_empty_result_error.phpt

+sqlsrv_close($conn);
+?>
+--EXPECTF--
+Return a nonempty result set:


why EXPECTF?

%s in the output may be either 'Microsoft' on Windows or 'unixODBC' on unix systems, and EXPECTREGEX seemed like overkill for one string in a long output like this.

david-puglielli · 2017-09-30T01:20:26Z

I'm not sure how useful testing with a regular SELECT statement would be, because I don't think the result set could ever be null in that case. At most you can have an empty result set with no rows but at least one column, or you select from a nonexistent column, which yields a different error message. These are not the same as a null result set, in which there are no rows and no columns, and it's why I needed to use the stored procedure in the tests to trigger a null result.

yitam

Approve assuming you have checked there's no regression

yitam · 2017-10-02T14:52:47Z

test/functional/pdo_sqlsrv/pdo_empty_result_error.phpt

+--TEST--
+Test that calling nextRowset() on an empty result set produces the correct error message. Fix for Github 507.
+--SKIPIF--
+<?php require('skipif.inc'); ?>


ahhh... I see! In the beginning you use DropTable and DropProc but later you simply use queries.

yitam

My oversight. Looks like you still have test failures. Please fix those.

…sphpsql into nextrowset-fix

coveralls · 2017-10-02T22:49:33Z

Coverage increased (+0.003%) to 73.276% when pulling 4349a3c on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

coveralls · 2017-10-02T23:30:26Z

Coverage increased (+0.003%) to 73.276% when pulling 70b20a2 on david-puglielli:nextrowset-fix into ca9ba63 on Microsoft:dev.

yitam

For the test that you had to fix the regex in order to pass, consider only checking for the first error (the PHP related error)

Added empty result set check to nextRowset()

83631cd

msftclas added the cla-already-signed label Sep 7, 2017

Added PHPT test

f4117a8

david-puglielli requested review from Hadis-Knj and yitam September 8, 2017 02:30

yitam reviewed Sep 8, 2017

View reviewed changes

Fixed bug for sqlsrv too, sqlsrv test added

8666ea6

david-puglielli added 3 commits September 21, 2017 18:27

Fixed test failures by moving logic back to stmt.cpp and pdo_stmt.cpp

7046086

Fixed compile bug

0d06dea

Update stmt.cpp

2c28d0b

Update sqlsrv_empty_result_error.phpt

f11c208

msftgits removed the cla-already-signed label Sep 26, 2017

david-puglielli added 5 commits September 26, 2017 15:04

Merge remote-tracking branch 'upstream/dev' into nextrowset-fix

be2be4b

Use core_sqlsrv_has_any_result to check for null results

270f2b4

fixed conflicts

1045ea4

Moved check to core_sqlsrv_next_result to avoid suppressing a differe…

68bb42b

…nt error message

Fixed errant variable name

66a12d3

david-puglielli added 4 commits September 27, 2017 17:01

Moved errors back to pdo_stmt and stmt.cpp

a749010

Fixed typos

d039cf6

Fixed typos again

7713b70

More tiny fixes

d56446c

Fixed tests and added code comments

3acab1a

Fixed test output

7315d44

yukiwongky reviewed Sep 29, 2017

View reviewed changes

Tweaked tests, added comments per code review comments

494527d

david-puglielli added 2 commits September 29, 2017 14:52

Update pdo_empty_result_error.phpt

0728a1d

Update sqlsrv_empty_result_error.phpt

dc6e2a3

david-puglielli added 2 commits September 29, 2017 16:17

Update pdo_stmt.cpp

9e92867

Update stmt.cpp

4674f58

yitam reviewed Sep 30, 2017

View reviewed changes

Minor fixes

4151f8f

yitam approved these changes Oct 2, 2017

View reviewed changes

yitam suggested changes Oct 2, 2017

View reviewed changes

david-puglielli added 4 commits October 2, 2017 13:47

Merge branch 'nextrowset-fix' of https://github.com/david-puglielli/m…

95f7675

…sphpsql into nextrowset-fix

Fixed EXPECTF test output

cc0d71d

Changed mssql-server-linux tag

6aa062f

Changed mssql-server-linux tag

4349a3c

Fixed regex output

70b20a2

yitam approved these changes Oct 3, 2017

View reviewed changes

yukiwongky approved these changes Oct 3, 2017

View reviewed changes

david-puglielli merged commit a1d6130 into microsoft:dev Oct 3, 2017

Added empty result set check to nextRowset() #520

Added empty result set check to nextRowset() #520

Conversation

david-puglielli commented Sep 7, 2017 • edited Loading

coveralls commented Sep 7, 2017

coveralls commented Sep 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-puglielli commented Sep 22, 2017 • edited Loading

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017

coveralls commented Sep 22, 2017

coveralls commented Sep 27, 2017

coveralls commented Sep 28, 2017

coveralls commented Sep 28, 2017

coveralls commented Sep 28, 2017 • edited Loading

yukiwongky Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukiwongky Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

coveralls commented Sep 29, 2017

yitam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-puglielli commented Sep 30, 2017

yitam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitam left a comment

Choose a reason for hiding this comment

coveralls commented Oct 2, 2017

coveralls commented Oct 2, 2017

yitam left a comment

Choose a reason for hiding this comment

david-puglielli commented Sep 7, 2017 •

edited

Loading

david-puglielli commented Sep 22, 2017 •

edited

Loading

coveralls commented Sep 28, 2017 •

edited

Loading

yukiwongky Sep 29, 2017 •

edited

Loading

yukiwongky Sep 29, 2017 •

edited

Loading